Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Extract Version #872

Closed
wants to merge 1 commit into from

Conversation

localheinz
Copy link
Contributor

This pull request

  • extracts Version as a value object

Related to #871.

$parts = explode('.', $version);
if (count($parts) > 1) {
return "$parts[0].$parts[1]";
function version_number_to_branch(string $value): ?string {
Copy link
Contributor Author

@localheinz localheinz Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could eventually disappear, and is only a starting point for using Version - also see #871.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments after a quick glance

Comment on lines 18 to 23
if (1 !== preg_match('/^(0|[1-9]\d*)$/', $value)) {
throw new \InvalidArgumentException(\sprintf(
'Value "%s" does not appear to be a valid value for a patch version.',
$value
));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ctype_digit() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctype_digit($value) returns true when $value has a leading 0: https://3v4l.org/XqGiU.

Comment on lines +47 to +60
public function major(): Major
{
return $this->major;
}

public function minor(): Minor
{
return $this->minor;
}

public function patch(): Patch
{
return $this->patch;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need those if we have readonly properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on whether we want to stick with the internal implementation: using accessors instead of public properties has the advantage that we can change the internals any time without changing the consumers.

I can adjust, though.

Comment on lines 34 to 37
throw new \InvalidArgumentException(\sprintf(
'Value "%s" does not appear to be a valid value for a version.',
$value
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new \InvalidArgumentException(\sprintf(
'Value "%s" does not appear to be a valid value for a version.',
$value
));
throw new \ValueError(\sprintf(
'Value "%s" does not appear to be a valid value for a version.',
$value
));

We could just throw a ValueError

return "$parts[0].$parts[1]";
function version_number_to_branch(string $value): ?string {
try {
$version = Release\Version::fromString($value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not $version = Version::fromString($value); What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted!

I have written about Avoiding imports and aliases in PHP; I generally try to avoid importing every single class from a specific namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I am rather against importing the namespace. IMHO every class should be imported unless it conflicts with another one. Only in case of conflicts, one should import namespace. Importing the full name makes for a cleaner code. The line is shorter, there's less visual bloat and my eyes can focus on what's really important: the name of the class. For the most part, I am not interested from which namespace the class comes. I can always hover over it to see the full namespace. The import list is maintained automatically by IDEs so I never look at it. If the import list is too long, it can always be collapsed using the alternative syntax.

Comment on lines +28 to +31
public function toString(): string
{
return $this->value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is completely unnecessary and also can be easily confused with the magic method __toString. This is the same as making the property public and directly accessing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a matter of preference - when I allow constructing an object from a simple type, I typically use a named constructor fromSimpleType() and a corresponding method toSimpleType(). This allows to construct an object from and cast it to other simple types when necessary, and hides the internals.

I have written about Naming constructors in PHP, where I attempt to explain my reasoning.

Also see https://github.com/php/web-php/pull/872/files#r1418528779.

return $this->patch;
}

public function toString(): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use the magic method __toString?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to explicitly invoke a method to cast a type to another type instead of relying on a magic method.

Comment on lines +16 to +26
public static function create(
Major $major,
Minor $minor,
Patch $patch,
): self {
return new self(
$major,
$minor,
$patch,
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make the constructor public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using one or more public named constructors and a single private constructor in conjunction with accessors instead of public properties allows us to change the internals of the class as needed.

From the call-site it does not matter whether someone invokes a method or accesses a property, but relying on a public property makes it harder to change internals.

For example, we could change the internals as follows and would not need to make any changes to call-sites:

diff --git a/src/Release/Version.php b/src/Release/Version.php
index 47975096..6b5664fd 100644
--- a/src/Release/Version.php
+++ b/src/Release/Version.php
@@ -6,11 +6,9 @@ namespace phpweb\Release;

 final readonly class Version
 {
-    private function __construct(
-        private Major $major,
-        private Minor $minor,
-        private Patch $patch,
-    ) {
+    private function __construct(private string $value)
+    {
+
     }

     public static function create(
@@ -18,11 +16,12 @@ final readonly class Version
         Minor $minor,
         Patch $patch,
     ): self {
-        return new self(
-            $major,
-            $minor,
-            $patch,
-        );
+        return new self(sprintf(
+            '%s.%s.%s',
+            $major->toString(),
+            $minor->toString(),
+            $patch->toString(),
+        ));
     }

     /**
@@ -37,35 +36,32 @@ final readonly class Version
             ));
         }

-        return new self(
-            Major::fromString($matches['major']),
-            Minor::fromString($matches['minor']),
-            Patch::fromString($matches['patch']),
-        );
+        return new self($value);
     }

     public function major(): Major
     {
-        return $this->major;
+        $parts = explode('.', $this->value);
+
+        return Major::fromString($parts[0]);
     }

     public function minor(): Minor
     {
-        return $this->minor;
+        $parts = explode('.', $this->value);
+
+        return Minor::fromString($parts[1]);
     }

     public function patch(): Patch
     {
-        return $this->patch;
+        $parts = explode('.', $this->value);
+
+        return Patch::fromString($parts[2]);
     }

     public function toString(): string
     {
-        return sprintf(
-            '%s.%s.%s',
-            $this->major->toString(),
-            $this->minor->toString(),
-            $this->patch->toString(),
-        );
+        return $this->value;
     }
 }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer the principles of KISS and YAGNI. When it comes to value objects, they should remain as simple as possible. But there's no reason to spend too much time on this as it feels more of a stylistic choice than anything else.

Comment on lines 19 to 27
$invalidValues = array_filter($values, static function (string $value): bool {
try {
Release\Major::fromString($value);
} catch (\InvalidArgumentException) {
return true;
}

return false;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too complex. Please simplify it to get rid of the higher-order function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a basic array_filter() usage, I don't see how this is complex.

Could it be just a foreach loop? Sure, but that just a preference at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Girgias @kamil-tekiela

Do you have any suggestions how to work with data providers in phpt tests?

This is what I am trying to achieve here.

Comment on lines 23 to 33
$invalidValues = array_filter($values, static function (string $value): bool {
try {
Release\Version::fromString($value);
} catch (\InvalidArgumentException) {
return true;
}

return false;
});

var_dump($invalidValues);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$invalidValues = array_filter($values, static function (string $value): bool {
try {
Release\Version::fromString($value);
} catch (\InvalidArgumentException) {
return true;
}
return false;
});
var_dump($invalidValues);
foreach($values as $label => $value) {
try {
Release\Version::fromString($value);
} catch (\InvalidArgumentException) {
echo $label . "\n";
echo $value . "\n";
}
}

@Girgias Isn't this easier to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamil-tekiela @Girgias

I can adjust if you prefer!

@localheinz localheinz force-pushed the feature/version branch 2 times, most recently from 7e23de6 to 3787bcb Compare December 7, 2023 08:17
@localheinz localheinz marked this pull request as draft December 7, 2023 14:40
@localheinz localheinz marked this pull request as ready for review December 7, 2023 14:42
@localheinz localheinz closed this Feb 12, 2024
@localheinz localheinz deleted the feature/version branch February 12, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants